Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed APIError: [403] when copying permissions #1515

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fixed APIError: [403] when copying permissions #1515

wants to merge 4 commits into from
+14 −1

Conversation

wobYY
Copy link

@wobYY wobYY commented Sep 24, 2024 •
edited
Loading

This PR fixes the bug raised in the issue #1507.

tox test (replaced the first part of the path with local_dir to remove personal information):

>> tox -e py
.pkg: _optional_hooks> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
.pkg: get_requires_for_build_sdist> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
.pkg: build_sdist> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
py: install_package> python -I -m pip install --force-reinstall --no-deps local_dir\gspread\.tox\.tmp\package\5\gspread-6.1.2.tar.gz
py: commands[0]> pytest tests/
======================================================================= test session starts ========================================================================
platform win32 -- Python 3.12.4, pytest-8.3.3, pluggy-1.5.0
cachedir: .tox\py\.pytest_cache
rootdir: local_dir\gspread
configfile: pyproject.toml
plugins: vcr-1.0.2
collected 141 items

tests\cell_test.py .......                                                                                                                                    [  4%]
tests\client_test.py .............                                                                                                                            [ 14%]
tests\spreadsheet_test.py .................                                                                                                                   [ 26%]
tests\utils_test.py ........................                                                                                                                  [ 43%]
tests\worksheet_test.py ................................................................................                                                      [100%]

========================================================================= warnings summary ========================================================================= 
tests\worksheet_test.py:39
  local_dir\gspread\tests\worksheet_test.py:39: PytestRemovedIn9Warning: Marks applied to fixtures have no effect
  See docs: https://docs.pytest.org/en/stable/deprecations.html#applying-a-mark-to-a-fixture-function
    @pytest.fixture(autouse=True)

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
================================================================== 141 passed, 1 warning in 3.87s ================================================================== 
.pkg: _exit> python local_dir\gspread\.venv\Lib\site-packages\pyproject_api\_backend.py True flit_core.buildapi
  py: OK (9.83=setup[4.83]+cmd[5.00] seconds)
  congratulations :) (9.97 seconds)

Also tested by copying a random spreadsheet in my workspace. For debugging I added a print in the if block:

                # .list_permissions() returns a list of permissions,
                # even the folder permissions if the file is in a shared folder.
                # We only want the permissions that are directly applied to the
                # spreadsheet file, i.e. 'writer', 'commenter' and 'reader'.
                perm_details = {
                    p_details.get("permissionType"): p_details.get("inherited")
                    for p_details in p.get("permissionDetails")
                }
                if p.get("role") in ("organizer", "fileOrganizer") and (
                    perm_details.get("file") or perm_details.get("member")
                ):
                    print(
                        f"`{p.get("id")[:3] + "..."}` has the `{p.get("role")}` role which is a folder permission, skipping..."
                    )
                    continue

                print(f"`{p.get("id")[:3] + "..."}` has the `{p.get("role")}` role, adding permission")

The result (screenshot):
011... has the organizer role which is a folder permission, skipping...
027... has the fileOrganizer role which is a folder permission, skipping...
029... has the commenter role, adding permission
033... has the fileOrganizer role which is a folder permission, skipping...
061... has the organizer role which is a folder permission, skipping...
083... has the fileOrganizer role which is a folder permission, skipping...
085... has the fileOrganizer role which is a folder permission, skipping...
087... has the organizer role which is a folder permission, skipping...
131... has the writer role, adding permission
134... has the fileOrganizer role which is a folder permission, skipping...
146... has the fileOrganizer role which is a folder permission, skipping...
152... has the organizer role which is a folder permission, skipping...

Correctly shared:
image

@alifeee
Copy link
Collaborator

alifeee commented Sep 24, 2024

hi ! thanks for the change :)

I have fixed a couple of linting issues. There is a typing issue that mypy does not like, that confuses me a little. @lavigne958 will be able to look at it

otherwise, I think your change makes sense. we will try to get the workflows fixed and then think about merging :)

@lavigne958 lavigne958 added the in progress Issue currently in progress by the assignee label Oct 10, 2024
@wobYY
Copy link
Author

wobYY commented Mar 5, 2025

Hey @lavigne958 I would like to help with fixing this and pushing it along. Could you share what should be tested and to what degree?

@alifeee
Copy link
Collaborator

alifeee commented Mar 26, 2025

hi :]

shame @lavigne958 is busy

I think the error has to do with the typing of the return value of this function(?), so mypy complains that you cannot use .get( as it is expecting a string

gspread/gspread/spreadsheet.py

Lines 546 to 548 in 6775c81

def list_permissions(self) -> List[Dict[str, Union[str, bool]]]:
"""Lists the spreadsheet's permissions."""
return self.client.list_permissions(self.id)

do you think it is possible to add a test that can fail/pass this bug, so we do not re-introduce it in future?

@wobYY
Copy link
Author

wobYY commented Mar 31, 2025 •
edited
Loading

I think the error has to do with the typing of the return value of this function(?), so mypy complains that you cannot use .get( as it is expecting a string

Yup, had to add an additional Typing definition - Found, fixed and tested. Linting isn't screaming anymore with an error.
https://github.com/wobYY/gspread/blob/52d0e1a7c327da82175426df9929c0ec72fe5375/gspread/spreadsheet.py#L546

do you think it is possible to add a test that can fail/pass this bug, so we do not re-introduce it in future?

I was thinking of adding something like below:
https://github.com/wobYY/gspread/blob/52d0e1a7c327da82175426df9929c0ec72fe5375/tests/client_test.py#L64-L84

This would test both copying of permissions and then on top of that, that being done in the Shared Drive. I'm testing on my company drive so my test service account doesn't have the permission to delete the file which is done at the end of the test. I'm awaiting my manager's approval on account getting a temporary full access so it can delete the files in that folder in the Shared Drive and successfully complete the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wobYY wobYY

Labels
in progress Issue currently in progress by the assignee
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wobYY @alifeee @lavigne958